operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.cpp (2)

490-510: Critical: Counters incremented before drop decision.

The counters bursts_dropped_critical_capacity_, total_bursts_dropped_, and pool_capacity_critical_events_ are incremented on lines 491-493 before calling should_drop_burst_adaptive() on line 510. When should_drop_burst_adaptive() returns false (e.g., during recovery when utilization >= pool_recovery_threshold_percent_ but still below pool_critical_threshold_percent_), the burst is not actually dropped but the counters were already incremented. This inflates drop statistics.

This is a duplicate of an issue flagged in previous reviews.

Move counter increments after confirming the drop:

   // Critical capacity - definitely drop with video-aware logic
   if (utilization < pool_critical_threshold_percent_) {
-    bursts_dropped_critical_capacity_++;
-    total_bursts_dropped_++;
-    pool_capacity_critical_events_++;
-
     auto now = std::chrono::steady_clock::now();
     auto time_since_last =
         std::chrono::duration_cast<std::chrono::milliseconds>(now - last_capacity_critical_time_)
             .count();

     if (time_since_last > 1000) {  // Log every second
       HOLOSCAN_LOG_ERROR(
           "CRITICAL: Pool capacity at {}% - dropping new bursts only (port={}, queue={})",
           utilization,
           port_id_,
           queue_id_);
       last_capacity_critical_time_ = now;
     }

     // In critical mode, use adaptive dropping but be more aggressive
-    return should_drop_burst_adaptive(utilization);
+    bool should_drop = should_drop_burst_adaptive(utilization);
+    if (should_drop) {
+      bursts_dropped_critical_capacity_++;
+      total_bursts_dropped_++;
+      pool_capacity_critical_events_++;
+    }
+    return should_drop;
   }

---

514-533: Critical: Counters incremented before drop decision.

Same issue as the critical capacity path - counters bursts_dropped_low_capacity_, total_bursts_dropped_, and pool_capacity_warnings_ are incremented on lines 515-517 before should_drop_burst_adaptive() determines whether to actually drop. This inflates statistics when bursts are not dropped.

This is a duplicate of an issue flagged in previous reviews.

Move counter increments after confirming the drop:

   // Low capacity - use adaptive dropping policies
   if (utilization < pool_low_threshold_percent_) {
-    bursts_dropped_low_capacity_++;
-    total_bursts_dropped_++;
-    pool_capacity_warnings_++;
-
     auto now = std::chrono::steady_clock::now();
     auto time_since_last =
         std::chrono::duration_cast<std::chrono::milliseconds>(now - last_capacity_warning_time_)
             .count();

     if (time_since_last > 5000) {  // Log every 5 seconds
       HOLOSCAN_LOG_WARN("LOW: Pool capacity at {}% - adaptive burst dropping (port={}, queue={})",
                         utilization,
                         port_id_,
                         queue_id_);
       last_capacity_warning_time_ = now;
     }

-    return should_drop_burst_adaptive(utilization);
+    bool should_drop = should_drop_burst_adaptive(utilization);
+    if (should_drop) {
+      bursts_dropped_low_capacity_++;
+      total_bursts_dropped_++;
+      pool_capacity_warnings_++;
+    }
+    return should_drop;
   }

operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/rivermax_config_manager.cpp (1)

564-588: Consider validating threshold relationships.

The parsing logic correctly handles optional configuration with sensible defaults. However, there's no validation that the thresholds maintain logical relationships (e.g., recovery_threshold > critical_threshold, low_threshold > critical_threshold). Invalid threshold configurations could lead to unexpected adaptive dropping behavior at runtime.

Consider adding validation after parsing:

   rivermax_rx_config.burst_pool_recovery_threshold_percent =
       burst_pool_config["recovery_threshold_percent"].as<uint32_t>(50);

+  // Validate threshold relationships
+  if (rivermax_rx_config.burst_pool_critical_threshold_percent >= 
+      rivermax_rx_config.burst_pool_low_threshold_percent) {
+    HOLOSCAN_LOG_WARN("Critical threshold ({}%) should be less than low threshold ({}%)",
+                      rivermax_rx_config.burst_pool_critical_threshold_percent,
+                      rivermax_rx_config.burst_pool_low_threshold_percent);
+  }
+  if (rivermax_rx_config.burst_pool_recovery_threshold_percent <= 
+      rivermax_rx_config.burst_pool_critical_threshold_percent) {
+    HOLOSCAN_LOG_WARN("Recovery threshold ({}%) should be greater than critical threshold ({}%)",
+                      rivermax_rx_config.burst_pool_recovery_threshold_percent,
+                      rivermax_rx_config.burst_pool_critical_threshold_percent);
+  }

   HOLOSCAN_LOG_INFO(

operators/advanced_network_media/common/adv_network_media_common.h (1)

27-39: Wrap macro in do-while(0) for safety.

The macro should use the do-while(0) pattern to prevent issues with if-else chains and ensure it behaves like a single statement.

Apply this diff:

-#define CUDA_TRY(stmt)                                                                  \
-  {                                                                                     \
-    cudaError_t cuda_status = stmt;                                                     \
-    if (cudaSuccess != cuda_status) {                                                   \
-      ANM_LOG_ERROR("Runtime call {} in line {} of file {} failed with '{}' ({})", \
-                         #stmt,                                                         \
-                         __LINE__,                                                      \
-                         __FILE__,                                                      \
-                         cudaGetErrorString(cuda_status),                               \
-                         static_cast<int>(cuda_status));                                \
-      throw std::runtime_error("CUDA operation failed");                                \
-    }                                                                                   \
-  }
+#define CUDA_TRY(stmt)                                                                  \
+  do {                                                                                  \
+    cudaError_t cuda_status = stmt;                                                     \
+    if (cudaSuccess != cuda_status) {                                                   \
+      ANM_LOG_ERROR("Runtime call {} in line {} of file {} failed with '{}' ({})",     \
+                         #stmt,                                                         \
+                         __LINE__,                                                      \
+                         __FILE__,                                                      \
+                         cudaGetErrorString(cuda_status),                               \
+                         static_cast<int>(cuda_status));                                \
+      throw std::runtime_error("CUDA operation failed");                                \
+    }                                                                                   \
+  } while (0)

applications/adv_networking_media_sender/cpp/adv_networking_media_sender.cpp (3)

17-17: Remove unused include.

The assert.h header is included but never used in this file.

Apply this diff:

-#include <assert.h>
 #include <sys/time.h>

---

43-46: Remove duplicate log statement.

Lines 43 and 46 log essentially identical messages about the Advanced Network manager type.

Apply this diff to remove the duplicate:

     HOLOSCAN_LOG_INFO("Using Advanced Network manager {}",
                       advanced_network::manager_type_to_string(mgr_type));
 
-    HOLOSCAN_LOG_INFO("Using ANO manager {}", advanced_network::manager_type_to_string(mgr_type));
     if (!tx_en) {

---

33-50: Consider returning error codes instead of calling exit().

Calling exit(1) from within compose() terminates the entire process abruptly. Consider propagating errors to main() via exceptions or return codes for more graceful shutdown and better testability.

operators/advanced_network_media/common/frame_buffer.h (1)

31-33: Remove using namespace from the header

Header-wide using namespace …; pollutes every translation unit that includes this file and risks name clashes. Please switch to explicit aliases for the specific symbols you need.

-using namespace rivermax::dev_kit::services;
-using namespace holoscan::advanced_network;
+using rivermax::dev_kit::services::IFrameBuffer;
+using holoscan::advanced_network::MemoryLocation;

operators/advanced_network/advanced_network/managers/rivermax/rivermax_mgr_impl/burst_manager.h (1)

548-562: Remove the extra total_bursts_dropped_++ increment.

should_drop_burst_due_to_capacity() already updates all drop counters (including total_bursts_dropped_). Incrementing again in enqueue_and_reset_current_burst() doubles every drop. Please drop the redundant increment so statistics stay accurate.

     if (should_drop_burst_due_to_capacity()) {
       rx_bursts_mempool_->enqueue_burst(cur_out_burst_);
       reset_current_burst();
-      total_bursts_dropped_++;
       return Status::SUCCESS;
     }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to applications/CMakeLists.txt : Register applications in applications/CMakeLists.txt using add_holohub_application(...) and optional DEPENDS OPERATORS

Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to workflow/CMakeLists.txt : Register workflows in workflow/CMakeLists.txt using add_holohub_application(...) and optional DEPENDS OPERATORS

Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to operators/CMakeLists.txt : Register operators in operators/CMakeLists.txt using add_holohub_operator(...) and optional DEPENDS EXTENSIONS

Learnt from: CR
Repo: nvidia-holoscan/holohub PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-22T16:16:16.226Z
Learning: Applies to pkg/CMakeLists.txt : Register packages in pkg/CMakeLists.txt using add_holohub_package(...)

applications/adv_networking_media_sender/python/adv_networking_media_sender.py (1)

33-96: Consider using logging.exception() for error logs in exception handlers.

When logging errors inside exception handlers, logging.exception() automatically includes the traceback, which aids debugging. This applies to lines 88-89, 92-93, and similar patterns throughout the file.

♻️ Example fix for one location

         except Exception as e2:
             if require_rx:
-                logger.error("RX is not enabled. Please enable RX in the config file.")
-                logger.error(f"Could not find advanced_network_media_rx configuration: {e2}")
+                logger.exception("RX is not enabled. Please enable RX in the config file.")
                 sys.exit(1)
             if require_tx:
-                logger.error("TX is not enabled. Please enable TX in the config file.")
-                logger.error(f"Could not find advanced_network_media_tx configuration: {e2}")
+                logger.exception("TX is not enabled. Please enable TX in the config file.")
                 sys.exit(1)

applications/adv_networking_media_player/python/adv_networking_media_player.py (1)

34-96: Consider extracting check_rx_tx_enabled to a shared module.

This function is duplicated between adv_networking_media_player.py and adv_networking_media_sender.py (see relevant code snippets). Consider extracting it to a shared utility module to reduce code duplication and ensure consistent behavior.

Additionally, within exception handlers (lines 88-94), consider using logger.exception() instead of logger.error() to automatically include stack traces, which aids debugging:

         except Exception as e2:
             if require_rx:
-                logger.error("RX is not enabled. Please enable RX in the config file.")
-                logger.error(f"Could not find advanced_network_media_rx configuration: {e2}")
+                logger.exception("RX is not enabled. Please enable RX in the config file.")
                 sys.exit(1)

operators/advanced_network_media/advanced_network_media_rx/adv_network_media_rx.h (1)

21-22: Include <memory> (and <cstdint>) explicitly for std::unique_ptr/uint16_t.

This avoids reliance on transitive includes and keeps the header self-sufficient.

♻️ Suggested include update

-#include <holoscan/holoscan.hpp>
+#include <cstdint>
+#include <memory>
+#include <holoscan/holoscan.hpp>

operators/advanced_network_media/advanced_network_media_rx/memory_copy_strategies.cpp (1)

176-203: Pointer arithmetic could overflow with buffer wraparound.

At Line 180, size_t pointer_diff = curr_ptr - prev_ptr assumes curr_ptr >= prev_ptr. If the buffer wraps around (detected later in detect_buffer_wraparound), this subtraction produces an unexpectedly large value due to unsigned underflow. The code handles this by detecting wraparound separately, but the is_exactly_contiguous check at Line 188 will fail silently with wrong values rather than explicitly handling the wraparound case.

Consider using signed arithmetic or checking for wraparound before computing the diff:

Suggested improvement

   for (size_t i = 1; i < collected_payloads_.size(); ++i) {
     uint8_t* prev_ptr = collected_payloads_[i - 1];
     uint8_t* curr_ptr = collected_payloads_[i];

-    size_t pointer_diff = curr_ptr - prev_ptr;
+    // Use signed arithmetic to detect backward jumps
+    ptrdiff_t signed_diff = curr_ptr - prev_ptr;
+    if (signed_diff <= 0) {
+      // Wraparound or backward pointer - not contiguous
+      is_exactly_contiguous = false;
+      is_stride_consistent = false;
+      break;
+    }
+    size_t pointer_diff = static_cast<size_t>(signed_diff);